Skip to content

Conversation

@Jes360
Copy link

@Jes360 Jes360 commented Sep 6, 2025

Description
Introduces a Redis-backed caching layer to improve performance of Graph API calls and reduce repeated network traffic.
Changes
• Middleware: CacheMiddleware caches successful GET responses when caching is enabled
• Cache API: Added /cache/status, /cache/clear, /cache/stats endpoints
• Cache Service: CacheService wraps Redis with get/set/delete/clear/stats, tracks hit/miss, and uses namespaced keys
Graph API:
• Graph Service now looks up users and /me in cache before calling Microsoft Graph
• Caches responses with TTL, truncates token in keys to avoid storing secrets.
• Added /graph/users and /graph/me endpoints
Integration: app/main.py updated to include middleware and new routers.
• Dependencies: Added redis and aioredis in pyproject.toml.
Testing
• Start Redis and run the API with CACHE_ENABLED=true.
• Verify TTL expiry works.
• Check /cache/stats for hit/miss counts.
• Use /cache/clear to flush Redis.
Planner Link
https://teams.microsoft.com/l/entity/com.microsoft.teamspace.tab.planner/mytasks?tenantId=d02378ec-1688-46d5-8540-1c28b5f470f6&webUrl=https%3A%2F%2Ftasks.teams.microsoft.com%2Fteamsui%2FpersonalApp%2Falltasklists&context=%7B%22subEntityId%22%3A%22%2Fv1%2Fassignedtome%2Fview%2Fboard%2Ftask%2FYZ9xTus_hEO4v7u7_hxt3MgAK1-X%22%7D

Copy link
Member

@dec1belPP dec1belPP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Jes360, thank you for the PR. Can you please retrofit this with @Jasi-with-noe implementation of the Graph service #1 that has been merged? This prevents duplicate graph services. Also clear up te conflicts. Thank you!

@Jes360
Copy link
Author

Jes360 commented Sep 17, 2025

  1. Removed the duplicate /graph/me and /graph/users endpoints.
  2. Kept the merged graph_router.
  3. Left /cache/status, /cache/clear, /cache/stats endpoints intact.
    Please let me know if further adjustments are needed.
    Thank you

@dec1belPP
Copy link
Member

dec1belPP commented Sep 19, 2025

Hey @Jes360, could you also resolve the conflicts? Thank you.

@Jes360 Jes360 requested a review from dec1belPP September 19, 2025 10:08
Copy link
Member

@dec1belPP dec1belPP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Jes360, thank you fixing up the conflicts. This PR successfully implements a Redis-based caching layer with middleware for GET response caching and management endpoints. While the core functionality meets the ticket requirements, there are several issues that must be addressed before merging.

1. Header Loss in Cached Responses

In lines 22 - 25 in cache_middleware.py, original response headers (Cache-Control, ETag, etc.) are discarded. This results in incorrect caching behavior, broken client expectations etc.

2. No Error Handling in Middleware

At line 20 in cache_middleware.py, Redis failures will crash all GET requests and result in a single point of failure.

3. Inconsistent Caching Logic

In lines 18 - 26 in cache_middleware.py, caching still occurs even when CACHE_ENABLED=False. This results in unexpected behavior.

4. Dependency Management

Both redis and aioredis are declared. Use redis with redis.asyncio instead. This reduces version conflicts and maintenance overhead.

5. Configuration Issues

Unused PREFIX constant in cache.py at line 6.

7. No Connection Management

Missing proper Redis connection management

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants